Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Rust] Support formParams and fix list-params. #1678

Merged
merged 2 commits into from
Dec 21, 2018

Conversation

mfarrugi
Copy link
Contributor

PR checklist

  • Read the contribution guidelines.
  • Ran the shell script under ./bin/ to update Petstore sample so that CIs can verify the change. (For instance, only need to run ./bin/{LANG}-petstore.sh and ./bin/security/{LANG}-petstore.sh if updating the {LANG} (e.g. php, ruby, python, etc) code generator or {LANG} client's mustache templates). Windows batch files can be found in .\bin\windows\.
  • Filed the PR against the correct branch: master, 3.4.x, 4.0.x. Default: master.
  • Copied the technical committee to review the pull request if your PR is targeting a particular programming language.

@frol @farcaller @bjgill


Description of the PR

Form params were previously unsupported, and list-params would produce code that didn't compile.

Tested against a private protocol that uses

        "consumes": [
          "application/x-www-form-urlencoded"
        ],

I'll follow up with the Petstore sample updates a little later.

Form params were previously unsupported, and list-params would produce code that didn't compile.
@wing328
Copy link
Member

wing328 commented Dec 20, 2018

@mfarrugi thanks for the PR. I've updated the samples via 0eac812

If no further feedback on this PR, I'll merge it tomorrow.

@wing328 wing328 merged commit 4a494b4 into OpenAPITools:master Dec 21, 2018
@mfarrugi
Copy link
Contributor Author

mfarrugi commented Dec 21, 2018

@wing328 thanks for taking care of the test updates.

@wing328
Copy link
Member

wing328 commented Dec 21, 2018

@mfarrugi you're welcome. Thanks again for the PR.

@wing328
Copy link
Member

wing328 commented Dec 21, 2018

My bad. The CI failed with the following:

warning: unused import: `super::models::*`
  --> src/apis/mod.rs:22:5
   |
22 | use super::models::*;
   |     ^^^^^^^^^^^^^^^^
   |
   = note: #[warn(unused_imports)] on by default
error[E0599]: no method named `to_string` found for type `std::vec::Vec<std::string::String>` in the current scope
   --> src/apis/pet_api.rs:115:71
    |
115 |             query.append_pair("status", &status.into_iter().map(|p| p.to_string()).collect::<Vec<String>>().join(",").to_string());
    |                                                                       ^^^^^^^^^
    |
    = note: the method `to_string` exists but the following trait bounds were not satisfied:
            `std::vec::Vec<std::string::String> : std::string::ToString`
            `[std::string::String] : std::string::ToString`
error[E0599]: no method named `to_string` found for type `std::vec::Vec<std::string::String>` in the current scope
   --> src/apis/pet_api.rs:147:67
    |
147 |             query.append_pair("tags", &tags.into_iter().map(|p| p.to_string()).collect::<Vec<String>>().join(",").to_string());
    |                                                                   ^^^^^^^^^
    |
    = note: the method `to_string` exists but the following trait bounds were not satisfied:
            `std::vec::Vec<std::string::String> : std::string::ToString`
            `[std::string::String] : std::string::ToString`
error[E0599]: no method named `to_string` found for type `models::File` in the current scope
   --> src/apis/pet_api.rs:286:28
    |
286 |             ("file", &file.to_string()),
    |                            ^^^^^^^^^
    | 
   ::: src/models/mod.rs:15:1
    |
15  | pub struct File;
    | ---------------- method `to_string` not found for this
    |
    = note: the method `to_string` exists but the following trait bounds were not satisfied:
            `models::File : std::string::ToString`
    = help: items from traits can only be used if the trait is implemented and in scope
    = note: the following trait defines an item `to_string`, perhaps you need to implement it:
            candidate #1: `std::string::ToString`
error: aborting due to 3 previous errors
For more information about this error, try `rustc --explain E0599`.
error: Could not compile `petstore_client`.

@mfarrugi can you please take a look when you've time?

@wing328
Copy link
Member

wing328 commented Dec 21, 2018

FYI. I've disabled the test for the time being: https://github.com/OpenAPITools/openapi-generator/blob/master/pom.xml

@mfarrugi
Copy link
Contributor Author

There are two problems:

  1. The Vec<> wrapping I added appears incorrect (not sure if my protocol didn't test this, or this is more complicated)
  2. File type is not handled.

I see that the hyper implementation uses unimplemented!(). I could do the same so that it will compile, but I'm not sure that it is better for a user to have a run-time error instead of a compile-time error.
Perhaps a better approach here is to make the return type of unimplemented methods !, so that they compile but are not easily callable.

I won't get to this for at least a week because of the holidays. The change should be reverted pending fixes.

@wing328
Copy link
Member

wing328 commented Dec 21, 2018

Please enjoy the holidays. We'll revert the change for the time being.

wing328 added a commit that referenced this pull request Dec 21, 2018
wing328 added a commit that referenced this pull request Dec 22, 2018
* Revert "Disable rust-reqwest petstore test"

This reverts commit 9e1cce2.

* Revert "Fix various CI issues (#1722)"

This reverts commit 77270a0.

* Revert "[Rust] Support formParams and fix list-params. (#1678)"

This reverts commit 4a494b4.
@wing328
Copy link
Member

wing328 commented Jan 2, 2019

@mfarrugi thanks for the contribution, which has been included in the 4.0.0-beta release: https://twitter.com/oas_generator/status/1079727020374806529.

Happy New Year and looking forward to more collaboration and contributions in 2019!

A-Joshi pushed a commit to ihsmarkitoss/openapi-generator that referenced this pull request Feb 27, 2019
* [Rust] Support formParams and fix list-params.

Form params were previously unsupported, and list-params would produce code that didn't compile.

* update rust samples
A-Joshi pushed a commit to ihsmarkitoss/openapi-generator that referenced this pull request Feb 27, 2019
…#1732)

* Revert "Disable rust-reqwest petstore test"

This reverts commit 9e1cce2.

* Revert "Fix various CI issues (OpenAPITools#1722)"

This reverts commit 77270a0.

* Revert "[Rust] Support formParams and fix list-params. (OpenAPITools#1678)"

This reverts commit 4a494b4.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants